Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for ptp_kvm to coreos-platform-chrony system-generator #2263

Merged
merged 2 commits into from
Apr 4, 2023

Conversation

karelvanhecke
Copy link
Contributor

This PR will allow CoreOS to make use of ptp_kvm when the underlying hypervisor supports it.

@jlebon
Copy link
Member

jlebon commented Feb 27, 2023

Thank you for opening this! I'm wondering though if we should do this by default on QEMU. Unlike on the other cloud platforms supported there, there's no guarantee that the host hypervisor clock is itself accurate. Maybe instead we should just document it in this section and leave it up to the user? I could also imagine Butane sugar for this to make it easier.

@jlebon
Copy link
Member

jlebon commented Feb 27, 2023

Some past discussions about ptp_kvm in https://bugzilla.redhat.com/show_bug.cgi?id=1780165#c10 and following. Quoting:

I don't think the ptp_kvm module is guaranteed to work. It's currently specific to the x86 arch and it works only if the host is using the tsc clocksource. I'm not sure how exactly it works in OpenStack.

There are few downsides to using the PHC refclock for guest synchronization. One is that it relies on the host to be properly synchronized. If you manage both, that's ok. There is also an issue that the NTP error estimates (root delay and root dispersion) are lost in the NTP->PHC->refclock conversion, so the guests underestimate the maximum error of their clock, which might be a problem in some applications.

@karelvanhecke
Copy link
Contributor Author

I don't think the ptp_kvm module is guaranteed to work. It's currently specific to the x86 arch and it works only if the host is using the tsc clocksource. I'm not sure how exactly it works in OpenStack.

It is true that ptp_kvm isn't guaranteed to work, though the module would simply fail to load and we can fall back to the default Chrony configuration. For example, deploying the image in a nested virtualization environment triggers this behavior.
Support has also improved since that discussion. If i'm not mistaken, arm/arm64 support for ptp_kvm has been implemented in the Linux kernel.

There are few downsides to using the PHC refclock for guest synchronization. One is that it relies on the host to be properly synchronized. If you manage both, that's ok.

A similar point could be made about the default Chrony behavior, which by default relies on the accuracy & availability of the NTP pool project.

There is also an issue that the NTP error estimates (root delay and root dispersion) are lost in the NTP->PHC->refclock conversion, so the guests underestimate the maximum error of their clock, which might be a problem in some applications.

The image shipped for Azure would suffer from a similar issue:

https://learn.microsoft.com/en-us/azure/virtual-machines/linux/time-sync

Azure hosts are synchronized to internal Microsoft time servers that take their time from Microsoft-owned Stratum 1 devices, with GPS antennas. Virtual machines in Azure can either depend on their host to pass the accurate time (host time) on to the VM or the VM can directly get time from a time server, or a combination of both.

Stratum information isn't automatically conveyed from the Azure host to the Linux guest.

Out of all these possible issues, the fact that we can't guarantee the accuracy of the hypervisor might be the biggest blocking factor. What if we deploy ptp_kvm + NTP pool servers combined? Chrony would detect the ptp_kvm clock as a falseticker when the hypervisor is incorrectly synchronized.

@jlebon
Copy link
Member

jlebon commented Mar 1, 2023

A similar point could be made about the default Chrony behavior, which by default relies on the accuracy & availability of the NTP pool project.

That's true, but IIUC there's no reason to think that the hypervisor clock has a higher chance of being accurate. The user may know it's the case for their particular environment, so it makes sense to let them opt into it.

What if we deploy ptp_kvm + NTP pool servers combined? Chrony would detect the ptp_kvm clock as a falseticker when the hypervisor is incorrectly synchronized.

That's interesting. To make sure I understand, what would be the advantage of this over only relying on the default NTP pool?

@karelvanhecke
Copy link
Contributor Author

That's interesting. To make sure I understand, what would be the advantage of this over only relying on the default NTP pool?

If the ptp_kvm source is deemed accurate, It'll keep polling all sources, but only use the results of ptp_kvm (from what I've seen when I tested this). All CoreOS instances on that virtualization platform will be synchronized with sub-millisecond accuracy with each other.

@cgwalters
Copy link
Member

Thanks so much for submitting this patch!

My PoV is by default we should trust the hypervisor. Anyone who doesn't want to would need to configure things.

@jlebon
Copy link
Member

jlebon commented Mar 3, 2023

That's interesting. To make sure I understand, what would be the advantage of this over only relying on the default NTP pool?

If the ptp_kvm source is deemed accurate, It'll keep polling all sources, but only use the results of ptp_kvm (from what I've seen when I tested this). All CoreOS instances on that virtualization platform will be synchronized with sub-millisecond accuracy with each other.

Thanks, that sounds reasonable.

My PoV is by default we should trust the hypervisor. Anyone who doesn't want to would need to configure things.

Are you talking about "trust" from a security perspective or as in "trust that the hypervisor clock is accurate"? I agree with the former but the latter is not clear to me. Though @karelvanhecke's latest suggestion is an interesting middle ground.

Interested to see what others think. Maybe we can discuss this in the next community meeting. I'll create a tracker ticket for it so it shows up in the filter (edit: filed coreos/fedora-coreos-tracker#1433).

@karelvanhecke BTW, do you have more information on the context around this? Is this fixing an issue for you?

@jlebon
Copy link
Member

jlebon commented Mar 8, 2023

@karelvanhecke We discussed this today in the community meeting (coreos/fedora-coreos-tracker#1433 (comment)). Would you be able to update this PR to implement:

What if we deploy ptp_kvm + NTP pool servers combined? Chrony would detect the ptp_kvm clock as a falseticker when the hypervisor is incorrectly synchronized.

?

@karelvanhecke
Copy link
Contributor Author

@jlebon rebased against latest testing-devel branch + enabled ntp pool in combination with ptp_kvm.

jlebon
jlebon previously approved these changes Mar 10, 2023
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM, but I'd like to manually test this before merging to sanity-check the falseticker case. Will try to do that next week.

@jlebon
Copy link
Member

jlebon commented Mar 23, 2023

I've verified this works as expected. Output of chronyc sources when the host clock is synchronized:

[root@cosa-devsh ~]# chronyc sources
MS Name/IP address         Stratum Poll Reach LastRx Last sample
===============================================================================
#* PHC0                          0   2   377     2  +8193ns[  -41us] +/-   34us
^- up2.com                       2   6   377    14   -413us[ -579us] +/-   70ms
^- 68-69-221-61.regn.hsdb.s>     1   6   377    13  -1281us[-1446us] +/-   18ms
^- ntp.nyy.ca                    1   6   377    13   +500us[ +334us] +/-   22ms
^- time.cloudflare.com           3   6   377    15   -532us[ -698us] +/-   16ms

The * means the source is selected for synchronization.

Output of chronyc sources when the host clock is inconsistent:

[root@cosa-devsh ~]# chronyc sources
MS Name/IP address         Stratum Poll Reach LastRx Last sample
===============================================================================
#x PHC0                          0   2   377     4  +91106h[+91106h] +/-   69us
^- up2.com                       2   6    17    35   -817us[ -817us] +/-   92ms
^* 68-69-221-61.regn.hsdb.s>     1   6    17    35   -169us[ -389us] +/-   19ms
^+ ntp.nyy.ca                    1   6    17    35   -306us[ -526us] +/-   20ms
^+ time.cloudflare.com           3   6    17    35   -218us[ -438us] +/-   17ms

The x means the source is flagged as a falseticker.

There's something I want to call out related to an additional change I pushed here. The ext.config.ntp.chrony.dhcp-propagation test was failing because ptp_kvm is supported in our CI builders, and so the chrony script neuters DHCP-provided NTP servers. This is consistent with other platforms which support platform-native clocks (AWS, Azure, GCP), but on QEMU something other than the default DHCP server could be used, and users may have relied on their own configured DHCP servers to pass in NTP servers. Now, those will be ignored. It's still possible to pick up DHCP-provided NTP servers, but would now require explicit configuration (as shown in the changes I made to the test).

@jlebon
Copy link
Member

jlebon commented Mar 31, 2023

Updated! I've added more context to the commit message and removed the changes to the ext.config.ntp.chrony.dhcp-propagation test since they're no longer necessary.

karelvanhecke and others added 2 commits April 4, 2023 16:32
KVM supports a `ptp_kvm` kernel module which allows the guest to query
the host clock for synchronization purposes.

Configure chrony to make use of it if it's available.

Don't disable the default pool; it'll be used by chrony to determine
whether the host clock is accurate or a falseticker.

Don't disable `PEERNTP` as we do on other platforms since we
historically haven't done so on QEMU and it's possible users have been
using their own DHCP servers (e.g. dnsmasq) to feed NTP configuration.

Closes: coreos/fedora-coreos-tracker#1433

Co-authored-by: Jonathan Lebon <jonathan@jlebon.com>
The chrony generator was converted to a regular systemd unit in 5e4f40c
("Move chrony config from generator to systemd service"). Let's rename
the test to reflect this.
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jlebon jlebon merged commit de8b656 into coreos:testing-devel Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants